suggestion: This scenario has three When-Then cycles (install first CE → assert available, apply dup CE → assert collision, update first CE → assert still colliding). Multiple When-Then blocks in a single Gherkin scenario is generally considered an anti-pattern — each scenario should specify one behavior, not a multi-step integration script.

Having multiple transitions makes it harder to diagnose which step failed and why, and the scenario title ("does not take over resources") only describes the final assertion.

Consider splitting into two scenarios:

1. Conflicting CE gets collision on install
2. Conflicting CE with higher revision still gets collision after update

They could share setup via a Background or a reusable Given step.

Also, this scenario involves updating the CE spec (adding DeploymentConfig) and asserting behavior after the update. That makes it a better fit for update.feature than install.feature. The earlier Copilot review also referenced this being in update.feature — was there a reason to move it here?

Moved the test to update.feature — agreed it fits better there.

On splitting: the "conflicting CE gets collision on install" case is already covered by the scenario right above ("Detect collision when a second ClusterExtension installs the same package after an upgrade"). The value of this scenario is the multi-step sequence: initial collision → spec change producing a higher-revision COS → still collision. Splitting would duplicate the first half of the existing test without adding coverage.

Also added assertions that the original CE remains Installed=True after both collision points, which was missing before.

suggestion: The double loop (test cases × methods) with a map[string][]string for expected results works, but it obscures the arrange-act-assert structure. A reader has to mentally cross-reference the map key with the method definition at the top to understand what any single test case asserts.

An alternative structure that would be clearer:

• Test listOtherActiveRevisions directly with explicit predicates (the real unit under test). This covers the shared filtering logic once.
• Add a couple of trivial tests for listSiblingRevisions and listPreviousRevisions that just verify they delegate with the correct predicate (e.g., one passes all, the other filters by revision number).

This separates "does the shared logic work" from "do the wrappers delegate correctly" and makes each test read as a clean given-when-then.

This is addressed — listOtherActiveRevisions and listSiblingRevisions have been removed. The current code only has listPreviousRevisions with a simple single-loop table-driven test. No more double loop or map cross-referencing.

suggestion: The name format fmt.Sprintf("%s/%s", m.name, tc.name) puts the method name first, but the outer loop iterates over test cases. This means Go test output interleaves methods:

listSiblingRevisions/should exclude self...
listPreviousRevisions/should exclude self...
listSiblingRevisions/should exclude archived...
listPreviousRevisions/should exclude archived...

Swapping to tc.name/m.name (or swapping the loop order) would group output by method, making it easier to scan all cases for one method when debugging a failure.

Also addressed — the double loop is gone entirely. Single loop over test cases calling listPreviousRevisions directly.

suggestion: The old test marked rev-2 (middle revision) as deleting with currentRev: "rev-3", which tested the case where a middle revision is deleting while both lower and higher revisions exist. The new test moves the deletion to rev-1 (lowest revision). Both validate the "skip deleting" filter, but the old arrangement was a more interesting interleaving — it verified that a deleting revision between two active ones is correctly excluded regardless of its position.

Consider keeping deletion on rev-2 and adjusting the expected results accordingly, or adding a second sub-case for middle-revision deletion.

Addressed — deletion is back on rev-2 (middle revision) with currentRev: "rev-3", matching the original arrangement.

suggestion: The godoc here explains why boxcutter needs siblings (avoiding false collisions during handover). That is caller context — it belongs at the call site in buildBoxcutterPhases where WithSiblingOwners is passed. The method itself just returns "all active non-self revisions for the same owner," which is already clear from its name and signature.

Keeping method-level docs focused on what the method does (not why a specific caller needs it) prevents the comment from becoming stale if another caller is added.

Addressed — listSiblingRevisions and WithSiblingOwners are gone. The remaining listPreviousRevisions godoc only describes what the method does, no caller context.

suggestion: The scenario asserts that the dup CE shows collision and that the higher revision still shows collision after update. But it doesn't verify that the original ${NAME} CE remains Available/Installed throughout the conflict. Adding something like And ClusterExtension "${NAME}" reports Installed as True after the dup's collision assertion would strengthen the test — confirming the original owner is unaffected is arguably the most important property to verify here.

Agreed — added ClusterExtension "ce-${SCENARIO_ID}" reports Installed as True after both collision points (initial collision and post-update higher-revision collision).

perhaps: owns?

Renamed to owns — ClusterExtension "${NAME}" owns 1 ClusterObjectSet.

could we assert here the full message?

The full message includes the phase index and colliding object details (GVK, namespace/name with scenario IDs), e.g.:

revision object collisions in phase 0
ConfigMap.v1 ns-<id>/test-configmap-<id> ...

Asserting the full message would be brittle since the object names are scenario-specific. The existing collision test at line 274 uses the same Message includes approach with just the fragment. Happy to add a more specific fragment like "revision object collisions in phase" if that helps.

given that we have applied in this tests two ClusterExtension it would be helpful here to distinguish visually which one reports the condition:

ClusterExtension "${NAME}-dup" reports Progressing as True with Reason Retrying and Message includes:

Added a comment above the assertion clarifying which CE is being checked: # The conflicting ClusterExtension (${NAME}-dup, now tracked as ${NAME}) should be retrying. The unnamed ClusterExtension reports steps use sc.clusterExtensionName which is the dup at that point, but that's not obvious to a reader.

A fully named variant like ClusterExtension "${NAME}-dup" reports ... with Reason ... and Message includes would require a new step definition that accepts name+type+status+reason+message — seemed like over-engineering for this one test.

should be

ClusterExtension "${NAME}" reports Installed as True

because it is very internal detail that NAME has ce-${SCENARIO_ID} convention.

Can't use ${NAME} here — after the dup CE is applied, ResourceIsApplied overwrites sc.clusterExtensionName to the dup's name, so ${NAME} resolves to the dup (which is retrying, not installed). We need ce-${SCENARIO_ID} to reference the original CE.

${SCENARIO_ID} is a first-class template variable exposed by the framework, and the ce- prefix is established in CreateScenarioContext (hooks.go:218). The existing TrackCurrentClusterExtensionForCleanup pattern has the same constraint — once you apply a second CE, you lose the ${NAME} reference to the first one.

An alternative would be adding a new template variable (e.g. ${ORIGINAL_NAME}) that TrackCurrentClusterExtensionForCleanup populates, but that feels like over-engineering for one test.

Can't use ${NAME} here — after the dup CE is applied, ResourceIsApplied overwrites sc.clusterExtensionName to the dup's name, so ${NAME} resolves to the dup (which is retrying, not installed). We need ce-${SCENARIO_ID} to reference the original CE.

We should update ResourceIsApplied or some other e2e test logic so that we can have clearer step descriptions.

which cluster extension report that?

The conflicting ClusterExtension (the dup, tracked as ${NAME} after ResourceIsApplied). The comment on the line above clarifies this: "The higher-revision COS (revision 2) should also collide, not take over resources." Same pattern as the first collision assertion where I added a clarifying comment in the latest push.

The conflicting ClusterExtension (the dup, tracked as ${NAME} after ResourceIsApplied). The comment on the line above clarifies this: "The higher-revision COS (revision 2) should also collide, not take over resources." Same pattern as the first collision assertion where I added a clarifying comment in the latest push.

For reader it should be better if we could have ClusterExtension ${NAME}-dup ...

I find the scenario title to describe a low-level implementation details - we should focus ourselves here on user visible change, something like "Cannot install further ClusterExtension referring already installed bundle".

Given that we are adding this test in the PR which is about upgrading boxcutter version, I am curious to understand if before this update we did not have such protection? If we did, then I would add this tests in a separate PR. Does boxcutter upgrade gives us this functionality automatically or we need to so something on our side to achieve it?

Renamed to "Cannot install a ClusterExtension that refers to an already installed bundle".

On the scope question: cross-CE collision protection is existing boxcutter behavior — it was there before this PR. This PR changes the sibling owners API from WithSiblingOwners (all active revisions from the same CE) to WithPreviousOwners (only lower-revision ones). That's a same-CE handover concern, not cross-CE. The e2e test was added as a regression test to confirm that cross-CE collision protection still works correctly after the API change, but it's not testing new functionality. Happy to split it out into a separate PR if you prefer.

Happy to split it out into a separate PR if you prefer.

It would make sense to me... and to merge that first, before upgrading the boxcutter.